-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PLE scan #132
base: main
Are you sure you want to change the base?
PLE scan #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Lane - thanks for all of this work. I hope it's functioning well in the lab soon.
There are some broad changes to your PR I think we should make. Overall, since this code is now becoming used beyond the qt3 lab, we should adopt some better engineering practices that I have ignored (which was a huge mistake on my part).
- I've installed flake8 in my IDE and it complains about various spacing around
=
and,
characters. These complaints exist through a lot of my original code (which you copied) and we should strive to fix all of this (not now, but at some point in time either incrementally or in one big code cleanup). So, if you see a lot of complaints from flake8/black or other linter that's based on PEP8, please fix those instead of copying my code verbatim. - The
src/applications
folder is deprecated. We should movePLEscan.py
tosrc/qt3utils/applications/qt3ple/main.py
. Perhaps ask the team though if another name besides qt3ple is warranted. - Add qt3ple (or other name) to pyproject.toml
- Type hinting -- please add this throughout all of your changes. This will really help future dev work by lab members. I've added type hints all throughout the latest version of the qt3scope and qt3scan applications.
- python modules should be all lower case(https://peps.python.org/pep-0008/#package-and-module-names).
PLEscanner.py
->ple_scanner.py
Reintroduced check_allowed_position back into VControl after incidental removal when inheritance was removed.
Removed extraneous functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good. Check my review for explicit feedback on specific code sections. I still need to go through part of the code, but these comments can get us started.
self.scanned_count_rate = [] | ||
|
||
|
||
class WavemeterAndScanner(PleScanner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a good implementation. if people perform multiple scans, it might be a good idea to implement an attribute so that users can choose between unidirectional or bidirectional scans. In the first case, the start and stop values are the same, but in the second case they flip for each consecutive scan (and the step size changes sign).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be easy to add. Jotting this down more as a note to myself: I should talk to Nick again about the sweeps while I'm thinking about this. There was something about the time of the downsweep for the unidirectional case which he said was a concern. I believe it had to do with hysteresis. I'll check so I can knock both of these out at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check in with Nick for the specific implementation on their setup, but for general use, I know that other people would be happy to have the bidirectional scan as an option.
Fixed issues with batch mode.
|
||
|
||
class MainApplicationView(): | ||
def __init__(self, main_frame, scan_range=[0, 2]) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How difficult would it be to change the default values on the GUI from the yaml? This idea should be a separate issue if we think we should go forward with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GUI is loaded before a YAML file is read, so difficult. But it should be easy to have it change the values once the YAML file is read. I think I can cook that up real fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that sounds good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, you need to implement a gui for the user to update the configuration through.
Adds functionality to perform a PLE scan.
With the GUI launched by PLEscan.py, you can set your voltage sweeps and read out the results from the SPCM